Skip to content

More Code Improvements#392

Merged
ExtremeFiretop merged 2 commits intoExtremeFiretop:WebFunfrom
Martinski4GitHub:WebFun
Jan 20, 2025
Merged

More Code Improvements#392
ExtremeFiretop merged 2 commits intoExtremeFiretop:WebFunfrom
Martinski4GitHub:WebFun

Conversation

@Martinski4GitHub
Copy link
Collaborator

  • Added/modified the shell script to have the option to keep/save the current configuration file when uninstalling the add-on. This can be useful when a user needs to uninstall all 3rd-party add-ons to do a full factory defaults reset and then do a reinstallation, or simply wants to save/keep the config file for re-installation on another (new?) router.

  • Added/modified the ASP file so the WebGUI page extracts the current version string from the shared custom settings instead of having a hard-coded value.

- Added/modified the shell script to have the option to save/keep the current configuration file when uninstalling the add-on.
  This can be useful when a user needs to uninstalls all 3rd-party add-ons to do a full factory defaults reset, or simply wants to save the config file for re-install on a another (new?) router.

- Added/modified the ASP file so the WebGUI page extracts the current version string from the shared custom settings instead of having a hard-coded string.

- Some coding improvements.
Minor formatting changes.
@ExtremeFiretop
Copy link
Owner

  • Added/modified the shell script to have the option to keep/save the current configuration file when uninstalling the add-on. This can be useful when a user needs to uninstall all 3rd-party add-ons to do a full factory defaults reset and then do a reinstallation, or simply wants to save/keep the config file for re-installation on another (new?) router.
  • Added/modified the ASP file so the WebGUI page extracts the current version string from the shared custom settings instead of having a hard-coded value.

Hi @Martinski4GitHub

Sorry for the delay, I had a busy weekend working for my side gig and also updating some 7 year old code for a Github project I still rely on lol.

Will start to review now!

@ExtremeFiretop
Copy link
Owner

  • Added/modified the ASP file so the WebGUI page extracts the current version string from the shared custom settings instead of having a hard-coded value.

Love this idea! Thank you!

  • Added/modified the shell script to have the option to keep/save the current configuration file when uninstalling the add-on. This can be useful when a user needs to uninstall all 3rd-party add-ons to do a full factory defaults reset and then do a reinstallation, or simply wants to save/keep the config file for re-installation on another (new?) router.

Very cool! Will be testing this

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 20, 2025

@Martinski4GitHub

Also; what is remaining on your to-do list before we start considering wrapping it up for public testing?

I would like to look into the update process from 1.3.9 and test it somehow fully.
Will need to poke at this a bit, right now I am still running the startup parameter manually after coping the script into the directory and running it to mount the WebUI. I expect the update process in 1.3.9 to handle this, but needs to be tested.

Secondly, I want to find a way to allow the user to start the update right away without needing to change the postpone period (similar to the shell script). That will take some thinking. We already have a pop-up prompt to confirm to check, so a second one to confirm to start the update anyways after the 60 second check seems a little janky to me which is the initial thought.

Also want to test and confirm the Changelog approval functionality. And the ROG and TUF build features (hidden or disabled by default)

@ExtremeFiretop ExtremeFiretop merged commit f1e2938 into ExtremeFiretop:WebFun Jan 20, 2025
1 check passed
@Martinski4GitHub
Copy link
Collaborator Author

  • Added/modified the shell script to have the option to keep/save the current configuration file when uninstalling the add-on. This can be useful when a user needs to uninstall all 3rd-party add-ons to do a full factory defaults reset and then do a reinstallation, or simply wants to save/keep the config file for re-installation on another (new?) router.
  • Added/modified the ASP file so the WebGUI page extracts the current version string from the shared custom settings instead of having a hard-coded value.

Hi @Martinski4GitHub

Sorry for the delay, I had a busy weekend working for my side gig and also updating some 7 year old code for a Github project I still rely on lol.

Ah, no worries; we all have our own personal stuff to take care of first so I never expect a response right away. If I can wait 6 months for JackYaz to respond to my messages, I can wait for you a couple of weeks :>) LOL!!!

@Martinski4GitHub
Copy link
Collaborator Author

@Martinski4GitHub

Also; what is remaining on your to-do list before we start considering wrapping it up for public testing?

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).
  4. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

If I have time, I may do the first 2 items on the list. However, my workload will start ramping up very soon. The first 2 weeks of January are usually slow at work so tried to do as much as I could during that time.

I would like to look into the update process from 1.3.9 and test it somehow fully. Will need to poke at this a bit, right now I am still running the startup parameter manually after coping the script into the directory and running it to mount the WebUI. I expect the update process in 1.3.9 to handle this, but needs to be tested.

Actually, the update process must be done from the 1.3.10 version because this is the one that has the latest updated code to install version 1.4.0 WebGUI as well. So, in fact, the 1.3.10 production release must be issued first (after testing & validating the upgrade process works fine) so users will be ready for the future 1.4.0 release.

Secondly, I want to find a way to allow the user to start the update right away without needing to change the postpone period (similar to the shell script). That will take some thinking. We already have a pop-up prompt to confirm to check, so a second one to confirm to start the update anyways after the 60 second check seems a little janky to me which is the initial thought.

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Also want to test and confirm the Changelog approval functionality. And the ROG and TUF build features (hidden or disabled by default)

Good points.

@ExtremeFiretop
Copy link
Owner

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).

All this has been noted and I agree.

  1. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

I actually have been actively testing with my node and Gnuton router, so far everything with Gnuton is good, and I only noticed that the node creates the symbolic links for the WebUi even though it doesn't mount it. I am not sure if we want to be doing this on the node since the WebUi isn't being mounted anyways.

If I have time, I may do the first 2 items on the list. However, my workload will start ramping up very soon. The first 2 weeks of January are usually slow at work so tried to do as much as I could during that time.

I would like to look into the update process from 1.3.9 and test it somehow fully. Will need to poke at this a bit, right now I am still running the startup parameter manually after coping the script into the directory and running it to mount the WebUI. I expect the update process in 1.3.9 to handle this, but needs to be tested.

Actually, the update process must be done from the 1.3.10 version because this is the one that has the latest updated code to install version 1.4.0 WebGUI as well. So, in fact, the 1.3.10 production release must be issued first (after testing & validating the upgrade process works fine) so users will be ready for the future 1.4.0 release.

Yes fair point, the testing needs to be done against the latest dev, not production! However we would need 1.3.10 to be pushed to production ahead of time.

Secondly, I want to find a way to allow the user to start the update right away without needing to change the postpone period (similar to the shell script). That will take some thinking. We already have a pop-up prompt to confirm to check, so a second one to confirm to start the update anyways after the 60 second check seems a little janky to me which is the initial thought.

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

It's not a bad idea and was a suggestion I considered actually so probably the best course of action if we can find a way to fit it in and make it look nice.

Also want to test and confirm the Changelog approval functionality. And the ROG and TUF build features (hidden or disabled by default)

Good points.

@Martinski4GitHub
Copy link
Collaborator Author

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).

All this has been noted and I agree.

  1. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

I actually have been actively testing with my node and Gnuton router, so far everything with Gnuton is good,

OK, it's very good that you've been actively testing all this time so we know if there are any issues as soon as we can. Good job, bud!!!

... and I only noticed that the node creates the symbolic links for the WebUi even though it doesn't mount it. I am not sure if we want to be doing this on the node since the WebUi isn't being mounted anyways.

Good catch!! I'll submit a PR in a couple of minutes to fix this.

@Martinski4GitHub
Copy link
Collaborator Author

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).

All this has been noted and I agree.

  1. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

I actually have been actively testing with my node and Gnuton router, so far everything with Gnuton is good, and I only noticed that the node creates the symbolic links for the WebUi even though it doesn't mount it. I am not sure if we want to be doing this on the node since the WebUi isn't being mounted anyways.

OK, I just submitted a PR that addresses the above issue WRT AiMesh nodes.
Please re-test whenever you get a chance.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 21, 2025

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).

All this has been noted and I agree.

  1. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

I actually have been actively testing with my node and Gnuton router, so far everything with Gnuton is good,

OK, it's very good that you've been actively testing all this time so we know if there are any issues as soon as we can. Good job, bud!!!

Exactly, any test we run I run across all three environments always. It's why it takes me a bit of time to get back results sometimes. It can be some work to copy the files around on all 3 devices and mount the webUIs (or try) and collect data for the newest result.

However, it's easiest to do it all together than to go back after the fact for the Gnuton and node and try to remember all the tests we did.

... and I only noticed that the node creates the symbolic links for the WebUi even though it doesn't mount it. I am not sure if we want to be doing this on the node since the WebUi isn't being mounted anyways.

Good catch!! I'll submit a PR in a couple of minutes to fix this.

Merci! It was a small thing, but something I noted to modify in my next commit if it wasn't fixed already haha

@TheS1R
Copy link

TheS1R commented Jan 21, 2025

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.

As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.

Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

@Martinski4GitHub
Copy link
Collaborator Author

This is what's left on my to-do list for 1.4.0 version:

  1. Show the current schedule for Automatic F/W Update checks.
  2. Show the current schedule for Automatic Script Update checks (if enabled).
  3. Eventually, have a dialog box for users to set up the cron schedule.
    (IMO, this could be a feature that might be postponed for a later release).

All this has been noted and I agree.

  1. Test & Validate the following:
  • Updating from 1.3.10 to 1.4.0 version.
  • Script 1.4.0 runs well AiMesh Nodes.
  • Script & WebGUI 1.4.0 run well on Gnuton version.

I cannot really help with AiMesh Nodes or Gnuton testing but hopefully, you'll have time to do that.

I actually have been actively testing with my node and Gnuton router, so far everything with Gnuton is good,

OK, it's very good that you've been actively testing all this time so we know if there are any issues as soon as we can. Good job, bud!!!

Exactly, any test we run I run across all three environments always. It's why it takes me a bit of time to get back results sometimes. It can be some work to copy the files around on all 3 devices and mount the webUIs (or try) and collect data for the newest result.

However, it's easiest to do it all together than to go back after the fact for the Gnuton and node and try to remember all the tests we did.

Excellent testing & validation strategy!! You were a couple of steps ahead all this time 👍 :>)

@Martinski4GitHub
Copy link
Collaborator Author

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.

As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.

Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

Very good testing and a very good catch!!
I have a suspicion of what the cause of the issue is. I'll commit a tentative fix for this timing issue shortly (before being summoned for dinner :>).

@Martinski4GitHub
Copy link
Collaborator Author

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.

As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.

Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

OK, I just committed a tentative fix for the timing issue. Whenever you get a chance, please reload/reinstall both the shell script and the ASP file before retesting.

@TheS1R
Copy link

TheS1R commented Jan 22, 2025

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.
As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.
Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

OK, I just committed a tentative fix for the timing issue. Whenever you get a chance, please reload/reinstall both the shell script and the ASP file before retesting.

Installed latest fixes. Retested — ten (10) invalid USB directories resulted in ten (10) error dialogs. Good to go!

@Martinski4GitHub
Copy link
Collaborator Author

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.
As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.
Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

OK, I just committed a tentative fix for the timing issue. Whenever you get a chance, please reload/reinstall both the shell script and the ASP file before retesting.

Installed latest fixes. Retested — ten (10) invalid USB directories resulted in ten (10) error dialogs. Good to go!

Great news!! Just as I was about to go offline - dinner time!!
Thanks for testing.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Jan 22, 2025

I have been regression testing with the latest changes, and I think that I have found a marginal timing issue with respect to validation of the Set Directory for F/W Updates field. If I enter an invalid USB directory and hit enter (or click Save), I sometimes do not see the error dialog — the invalid value is reverted to the original as it should. I cannot come to any conclusion as to when it will or will not present the error dialog.

As a test for frequency, I just edited the USB location ten (10) times with invalid data, and three of the ten (3/10) times failed to display the error dialog. Each time that the error dialog was missing, the web page did not scroll down to show the Advanced Options with the Set Directory for F/W Updates field in view, but rather stayed at the top of the page.

Based upon how inconsistent this is, it most likely was an existing router-specific (GT-BE98 Pro) bug that was not introduced by the recent changes.

I just attempted to recreate this specific issue with the existing version (before martinskis fix) and was unable too.
But considering you said you cannot come to any conclusion as to when it will or will not present the error dialog, I guess that doesn't mean much that I was unable too.. (Randomly displayed)

Happy to hear that Martinskis fix resolved it for you though! I'll merge it in :)

@ExtremeFiretop
Copy link
Owner

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:

image

Or Option B?:

image

@TheS1R
Copy link

TheS1R commented Jan 23, 2025

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:

image

Or Option B?:

image

Option C: Similar to Option A, but move the label/checkbox to the right of the F/W Update Check button. Feels more intuitive to me...

@ExtremeFiretop
Copy link
Owner

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:
image
Or Option B?:
image

Option C: Similar to Option A, but move the label/checkbox to the right of the F/W Update Check button. Feels more intuitive to me...

We can do that but it requires making more space between the buttons. The reason I picked the left side is because it had more space. If I space out all the buttons though I including Approve Changelog and Uninstall, we could have the space without putting things out of proportion

@Martinski4GitHub
Copy link
Collaborator Author

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:
image
Or Option B?:
image

Option C: Similar to Option A, but move the label/checkbox to the right of the F/W Update Check button. Feels more intuitive to me...

We can do that but it requires making more space between the buttons. The reason I picked the left side is because it had more space. If I space out all the buttons though I including Approve Changelog and Uninstall, we could have the space without putting things out of proportion

How about this?

MerlinAU_WebGUI_Checkmarks

It looks clean & intuitive to me as each checkmark is directly associated with the button right above so the user can enable/disable it (as needed) and then just click on the button.

I'm currently coding the rest of the implementation to perform the actions required.

@ExtremeFiretop
Copy link
Owner

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:
image
Or Option B?:
image

Option C: Similar to Option A, but move the label/checkbox to the right of the F/W Update Check button. Feels more intuitive to me...

We can do that but it requires making more space between the buttons. The reason I picked the left side is because it had more space. If I space out all the buttons though I including Approve Changelog and Uninstall, we could have the space without putting things out of proportion

How about this?

MerlinAU_WebGUI_Checkmarks

It looks clean & intuitive to me as each checkmark is directly associated with the button right above so the user can enable/disable it (as needed) and then just click on the button.

I'm currently coding the rest of the implementation to perform the actions required.

I didn't try below the button! That looks good to me!!

@Martinski4GitHub
Copy link
Collaborator Author

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

...

...

We can do that but it requires making more space between the buttons. The reason I picked the left side is because it had more space. If I space out all the buttons though I including Approve Changelog and Uninstall, we could have the space without putting things out of proportion

How about this?
MerlinAU_WebGUI_Checkmarks
It looks clean & intuitive to me as each checkmark is directly associated with the button right above so the user can enable/disable it (as needed) and then just click on the button.
I'm currently coding the rest of the implementation to perform the actions required.

I didn't try below the button! That looks good to me!!

OK, I submitted a new PR #395 implementing the checkmarks functionality.
Please take a look and validate when you get a chance.

@TheS1R
Copy link

TheS1R commented Jan 23, 2025

What about adding a checkmark next to or below the "F/W Update Check" button to indicate that you want to ignore or bypass the postponement period and do the update right away if available?

Option A:
image
Or Option B?:
image

Option C: Similar to Option A, but move the label/checkbox to the right of the F/W Update Check button. Feels more intuitive to me...

We can do that but it requires making more space between the buttons. The reason I picked the left side is because it had more space. If I space out all the buttons though I including Approve Changelog and Uninstall, we could have the space without putting things out of proportion

How about this?
MerlinAU_WebGUI_Checkmarks
It looks clean & intuitive to me as each checkmark is directly associated with the button right above so the user can enable/disable it (as needed) and then just click on the button.
I'm currently coding the rest of the implementation to perform the actions required.

I didn't try below the button! That looks good to me!!

Looks perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants